Skip to content

Add beta_cdf and inverse_beta_cdf math functions.#11981

Closed
Vayu wants to merge 4 commits intoprestodb:masterfrom
Vayu:add-beta-cdf-function
Closed

Add beta_cdf and inverse_beta_cdf math functions.#11981
Vayu wants to merge 4 commits intoprestodb:masterfrom
Vayu:add-beta-cdf-function

Conversation

@Vayu
Copy link
Copy Markdown

@Vayu Vayu commented Nov 26, 2018

These are useful for calculating confidence intervals of Bernoulli trails.
https://en.wikipedia.org/wiki/Binomial_proportion_confidence_interval#Jeffreys_interval

Copy link
Copy Markdown
Contributor

@martint martint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The syntax looks good. It's consistent with the normal distribution variants.

Comment thread presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java Outdated
Comment thread presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java Outdated
Use BetaDistribution constructor with explicit RandomGenerator = null
 since we don't need it here.
@Vayu
Copy link
Copy Markdown
Author

Vayu commented Nov 29, 2018

@martint addressed comments

Comment thread presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java Outdated
Comment thread presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java Outdated
Comment thread presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java Outdated
@Vayu
Copy link
Copy Markdown
Author

Vayu commented Nov 30, 2018

clarified error messages

@Vayu
Copy link
Copy Markdown
Author

Vayu commented Dec 3, 2018

@martint improved error messages, can we merge that too?

@martint
Copy link
Copy Markdown
Contributor

martint commented Dec 7, 2018

improved error messages, can we merge that too?

I'll do that after the current release goes out.

@martint martint self-assigned this Dec 7, 2018
@Vayu
Copy link
Copy Markdown
Author

Vayu commented Mar 14, 2019

@mbasmanova @rschlussel @rongrong this PR got forgotten, it has improved error messages for beta_cdf and inverse_beta_cdf. Can we merge it?

@mbasmanova
Copy link
Copy Markdown
Contributor

@Vayu Could you rebase to the top of master and resolve conflicts. I'll review and merge then.

@mbasmanova
Copy link
Copy Markdown
Contributor

@Vayu I'm seeing 5 commits, but I think there should be only one or two. Please, squash commits.

@mbasmanova mbasmanova self-assigned this Mar 15, 2019
@mbasmanova
Copy link
Copy Markdown
Contributor

@Vayu, the final diff looks wrong. It only includes error message improvements, but doesn't show any new functions. Would you double check?

@rongrong
Copy link
Copy Markdown
Contributor

@Vayu you might want to rebase your changes on top of the current master, rather than doing a merge. Please merge all your changes to 1 single commit. You can do git rebase -i locally to merge the commits together, then git push -f origin add-beta-cdf-function to update the branch.

@mbasmanova
Copy link
Copy Markdown
Contributor

@Vayu ping

@Vayu
Copy link
Copy Markdown
Author

Vayu commented Mar 22, 2019

@mbasmanova the final diff has only error messages because everything before that is already merged into release branch, so the merge should only pick the last commit. Do you still want me to rebase and squash though?

@mbasmanova
Copy link
Copy Markdown
Contributor

@Vayu, understood. Please, rebase, squash commits if there will be more than one left and review the commit message to make sure it accurately reflects the changes.

@mbasmanova
Copy link
Copy Markdown
Contributor

mbasmanova commented Apr 2, 2019

@Vayu Valery, are you still interested in merging this PR? If so, would you rebase, resolve conflicts and squash commits?

@Vayu
Copy link
Copy Markdown
Author

Vayu commented Apr 9, 2019

moved follow-up to #12621

@Vayu Vayu closed this Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants